Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support @Meta annotation #4378

Merged
merged 3 commits into from
Jul 10, 2018
Merged

Support @Meta annotation #4378

merged 3 commits into from
Jul 10, 2018

Conversation

ZheSun88
Copy link
Contributor

This repeatable annotation can be used to create customized meta tags

This repeatable annotation can be used to create customized meta tags
import java.lang.annotation.Target;

/**
* Defines a custom tag and its content that will be added to the HTML of the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

custom tag?

As I understand it's a meta tag but with the name and content specified here.
This javadoc is confusing for me.
Would be good to rephrase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rephrased.

}
if (illegalValue) {
throw new IllegalStateException(
"Meta tag contains illegal value on name or content attribute.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which meta tag?
If there are many of them then it's hard to understand which one this message is about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the Exception message to clarify that the error is from the annotation

/**
* Returns the map which contains name and content of the customized meta
* tag for the target route chain that was navigated to, specified with
* {@link Meta} on the {@link Route} class or the {@link ParentLayout} of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Route and ParentLayout participate somehow in this code?
via the context.getPageConfigurationAnnotations(Meta.class) call ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have Meta for a class without Route or ParentLayout?
In any case it would be good to have a test for such case: it should either throw or work or do whatever expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it stated in the enhancement ticket, this annotation should work under the same context with Viewport, so i just took the javadoc from there.

Added one more test for using annotation without route, which expect to throw the InvalidRouteConfigurationException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the Route participates in getting the context from BootstrapHandler..
Actually, i am not sure about adding this test, as this should not be the test case for this annotation only. imo, all the annotation we used in this way should have similar test, like @Viewport @bodysize ...

@denis-anisimov denis-anisimov merged commit e5b5ccd into master Jul 10, 2018
@denis-anisimov denis-anisimov deleted the 3248-add-meta branch July 10, 2018 11:15
@gilberto-torrezan gilberto-torrezan added this to the 1.1.0.alpha1 milestone Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants